Skip to content

Add warnings for reserved Python keywords in interface members#96

Merged
ivanpauno merged 3 commits intoros2:masterfrom
samiamlabs:reserved_keyword_warnings
Feb 11, 2020
Merged

Add warnings for reserved Python keywords in interface members#96
ivanpauno merged 3 commits intoros2:masterfrom
samiamlabs:reserved_keyword_warnings

Conversation

@samiamlabs
Copy link
Contributor

@samiamlabs samiamlabs commented Dec 4, 2019

This is a temporary fix related to issue #95. Until a language-specific name mangling is implemented to avoid keywords as described in ros2/design#172, a warning would be nice.

I can add stderr prints for services and actions also if this pull request seems like a good idea.

Example build output from colcon:

Starting >>> dredgebot_msgs
Starting >>> rosidl_generator_py
Finished <<< rosidl_generator_py [7.05s]                                                               
[Processing: dredgebot_msgs]                             
--- stderr: dredgebot_msgs                                  
Member name "from" in the message "LatLonLineSegment" is a reserved keyword in Python and is not supported at the moment. Please use a different name.
---
Finished <<< dredgebot_msgs [39.4s]

Summary: 2 packages finished [39.5s]
  1 package had stderr output: dredgebot_msgs

@samiamlabs samiamlabs force-pushed the reserved_keyword_warnings branch from e32b31e to 4a77ab9 Compare December 4, 2019 11:30
@wjwwood
Copy link
Member

wjwwood commented Jan 2, 2020

@samiamlabs is this still a WIP?

@samiamlabs
Copy link
Contributor Author

samiamlabs commented Jan 2, 2020

I have not added error-prints for services or actions yet. If you think this PR makes sense I can fix that before merging.

Alternatively, we could just merge this PR and I can make a new one for actions and services.

wjwwood
wjwwood requested changes Jan 2, 2020
View reviewed changes
@wjwwood
Copy link
Member

wjwwood commented Jan 2, 2020

I'd advise you to fix it up to check in actions and services too before removing the WIP, but that's up to you. I do think this pr makes sense in general. @dirk-thomas do you think so too? My one concern is that this will warn on messages that already exist and cannot easily be changed, but maybe we already fixed that. I don't remember.

@wjwwood
Copy link
Member

wjwwood commented Jan 16, 2020

ping @dirk-thomas

Also, @samiamlabs do you think you'll have time to follow up on this? No pressure but it would help us triaging things to know.

@ivanpauno ivanpauno added enhancement New feature or request in review Waiting for review (Kanban column) labels Jan 16, 2020
@wjwwood wjwwood removed their assignment Jan 16, 2020
@samiamlabs
Copy link
Contributor Author

samiamlabs commented Jan 17, 2020

I will try to find some time to work on this at the start of next week :)

@samiamlabs samiamlabs force-pushed the reserved_keyword_warnings branch from c4451cf to a2e61ce Compare January 22, 2020 22:19
@samiamlabs samiamlabs changed the title [WIP] Add warnings for reserved Python keywords in interface members Add warnings for reserved Python keywords in interface members Jan 22, 2020
@samiamlabs
Copy link
Contributor Author

samiamlabs commented Jan 22, 2020

Finally got around to do some more work on this.
Here is an example build output:

[Processing: rosidl_python_test_interfaces]                              
--- stderr: rosidl_python_test_interfaces                                  
Member name 'from' in the message 'ReservedKeywords' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'yield' in the message 'ReservedKeywords' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'is' in the service request 'ReservedKeywordsRequest' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'lambda' in the service response 'ReservedKeywordsResponse' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'nonlocal' in the action feedback 'ReservedKeywordsFeedback' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'global' in the action goal 'ReservedKeywordsGoal' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
Member name 'pass' in the action result 'ReservedKeywordsResult' is a reserved keyword in Python and is not supported at the moment. Please use a different name.
---
Finished <<< rosidl_python_test_interfaces [30.7s]

I also set up a VSCode "Dev Container" for the nightly ROS2 docker image in case anyone is interested: https://github.com/samiamlabs/ros2-nightly-vscode/tree/reserved-keyword-warnings

wjwwood
wjwwood approved these changes Jan 24, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I'll run CI.

@wjwwood
Copy link
Member

wjwwood commented Jan 24, 2020

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

ivanpauno commented Jan 30, 2020

@wjwwood friendly ping. Is this ready? Does something have to be addressed?

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2020

There are unresolved linter failures, see: #96 (comment)

@samiamlabs
Copy link
Contributor Author

samiamlabs commented Feb 6, 2020

I pushed a fix for the linter issues a while back @wjwwood
Would be great if you can run CI again to make sure the errors are gone :)

@ivanpauno
Copy link
Member

ivanpauno commented Feb 6, 2020

CI (only fastrtps):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno requested a review from wjwwood February 6, 2020 12:45
@samiamlabs
Copy link
Contributor Author

samiamlabs commented Feb 6, 2020

We noticed that using "from" in a message caused a runtime syntax error for python when importing the message.
Some of the reserved keywords can probably be accessed using getattr without issues like this.
See #95

@ivanpauno
Copy link
Member

ivanpauno commented Feb 6, 2020

We noticed that using "from" in a message caused a runtime syntax error for python when importing the message.
Some of the reserved keywords can probably be accessed using getattr without issues like this.
See #95

IMO, that could be fixed if we use everywhere in the generated files getattr instead of dot syntax.
If the current generated files raise an error when importing, we should rather fail than log a warning (maybe it's a bad idea, because it can generate problems in existing messages). The other option is to fix the places that makes the imported message to fail (using getattr), and just log a warning indicating the usage of a python keyword.

@ivanpauno
Copy link
Member

ivanpauno commented Feb 7, 2020

@samiamlabs Can you rebase? Most of the test failures are resolved in master.

@samiamlabs
Add warning for keyword in message member
a488532 
Signed-off-by: Samuel <samuel@dynorobotics.se>
@samiamlabs
Add keyword warnings for services and actions
69a7a28 
Signed-off-by: Samuel Lindgren <samuel@dynorobotics.se>
@samiamlabs
Fix import order
4afae40 
Signed-off-by: Samuel Lindgren <samuel@dynorobotics.se>
@samiamlabs samiamlabs force-pushed the reserved_keyword_warnings branch from 612f361 to 4afae40 Compare February 10, 2020 15:02
@samiamlabs
Copy link
Contributor Author

samiamlabs commented Feb 10, 2020

I pushed a rebase with upstream master just now. Hope that helps with CI :)

@ivanpauno
Copy link
Member

ivanpauno commented Feb 10, 2020

I pushed a rebase with upstream master just now. Hope that helps with CI :)

Thanks! Another round of CI:

  • Linux Build Status

wjwwood
wjwwood approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @ivanpauno should we merge this or run CI again (since the interactive markers issues have been fixed)?

@ivanpauno
Copy link
Member

ivanpauno commented Feb 11, 2020

CI failure seems unrelated, merging!

@ivanpauno ivanpauno merged commit f19748a into ros2:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request in review Waiting for review (Kanban column)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samiamlabs @wjwwood @ivanpauno